fix(helm): honor release namespace instead of hardcoded ns-aiq#309
fix(helm): honor release namespace instead of hardcoded ns-aiq#309e-minguez wants to merge 1 commit into
Conversation
The aiq.namespace helper rendered ns-<appname> (e.g. ns-aiq) for every resource, ignoring the install namespace. This broke helm install -n and GitOps operators (ArgoCD, Fleet) that don't pre-create ns-aiq, failing with 'namespaces "ns-aiq" not found'. Default the helper to .Release.Namespace and add an optional namespace.name override to pin a specific namespace. Align namespace.yaml to use the same helper. Bump chart versions (aiq 0.0.4->0.0.5, aiq2-web 2.0.0->2.0.1) and refresh the vendored dependency package. Fixes NVIDIA-AI-Blueprints#290 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: e-minguez <e.minguez@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**⚙️ CodeRabbit configuration file
Files:
{deploy/**,configs/**}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (5)
WalkthroughUpdates the ChangesNamespace resolution fix and version bumps
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: Fixes Sequence Diagram(s)sequenceDiagram
participant Helm as Helm CLI
participant NamespaceTpl as namespace.yaml
participant Helper as aiq.namespace helper
participant Values as values.yaml
Helm->>NamespaceTpl: render Namespace resource
NamespaceTpl->>Helper: include "aiq.namespace" .
Helper->>Values: check namespace.name
alt namespace.name set
Helper-->>NamespaceTpl: return namespace.name
else namespace.name empty
Helper-->>NamespaceTpl: return .Release.Namespace
end
NamespaceTpl-->>Helm: rendered metadata.name
Suggested reviewers: aiq maintainers familiar with Helm chart deployment targets and GitOps operator compatibility. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test ead91f3 |
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
Please turn #290's reproduction into a regression test. Render the packaged umbrella with -n my-namespace and assert every namespaced resource uses my-namespace. helm lint passes on the currently broken chart, so it does not protect this contract. The test should run with Helm 3 because that is the reported and documented environment.
| kind: Namespace | ||
| metadata: | ||
| name: {{ .Release.Namespace }} | ||
| name: {{ include "aiq.namespace" . }} |
There was a problem hiding this comment.
#290 only requires resources to honor .Release.Namespace. Adding namespace.name introduces a second namespace authority and makes namespace.create retarget a chart-owned Namespace, which creates unsafe upgrade semantics. Can we keep this fix narrow by always using .Release.Namespace, or explicitly define and protect the cross-namespace lifecycle?
| description: AI-Q - WEB Profile () | ||
| type: application | ||
| version: 2.0.0 | ||
| version: 2.0.1 |
There was a problem hiding this comment.
2 participants
Overview
The aiq.namespace helper rendered ns- (e.g. ns-aiq) for every resource, ignoring the install namespace. This broke helm install -n and GitOps operators (ArgoCD, Fleet) that don't pre-create ns-aiq, failing with 'namespaces "ns-aiq" not found'.
Default the helper to .Release.Namespace and add an optional namespace.name override to pin a specific namespace. Align namespace.yaml to use the same helper. Bump chart versions (aiq 0.0.4->0.0.5, aiq2-web 2.0.0->2.0.1) and refresh the vendored dependency package.
Validation
git commit -sor an equivalent sign-off.Where should reviewers start?
Related Issues
Fixes #290
Summary by CodeRabbit
New Features
Bug Fixes